Skip to content

Escape the spaces in the monster name - #509

Merged
erwan-joly merged 1 commit into
masterfrom
feat/escape-last-string-field
Aug 30, 2026
Merged

Escape the spaces in the monster name#509
erwan-joly merged 1 commit into
masterfrom
feat/escape-last-string-field

Conversation

@erwan-joly

@erwan-joly erwan-joly commented Aug 30, 2026

Copy link
Copy Markdown
Contributor

What

EInfoNpcMonsterPacket.Name is [PacketIndex(25)] — the last field — and a last field keeps its spaces, so free text like a chat message survives the trip. A monster name is a value, not free text, and 929 of the 1109 monster-info lines in a capture carry a name with a space in it.

EscapeSpaces already exists for exactly this, and GInfoPacket, MlInfoBrPacket and MlintroPacket declare it. This one did not, so consumers were left calling Replace(' ', '^') themselves before handing the value over — which is what NosCoreIO/NosCore#2365 was about to ship.

One attribute argument, and <Version> 21.1.1 → 21.1.2.

Testing

  • SerializeEInfoNpcMonsterPacketEscapesTheName asserts the serialised tail reads -1 Fire^Cannoneer. Written red first: without the attribute it fails on the raw Fire Cannoneer.
  • dotnet test: 128 tests, all green.
  • Not played in game — this is the wire shape; the client-side effect is the info card drawing the full name instead of stopping at the space.

Summary by CodeRabbit

  • Bug Fixes

    • Fixed monster names containing spaces so they are correctly escaped when transmitted, improving packet parsing and display reliability.
  • Maintenance

    • Updated the packet package version to 21.1.2.
  • Tests

    • Added coverage to verify correct serialization of monster names with spaces.

Name ends the e_info line, and a last field keeps its spaces so free text
like a chat message survives. A monster name is a value, not free text, and
EscapeSpaces already exists for exactly that - GInfoPacket, MlInfoBrPacket
and MlintroPacket declare it. This one did not, so consumers were left to
call Replace(' ', '^') themselves before handing the value over.

929 of the 1109 monster-info lines in a capture carry a name with a space.

Test asserts the serialised tail, and fails on the raw name without the
attribute.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Aug 30, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: d768a5f9-2453-4713-972e-fa848056135d

📥 Commits

Reviewing files that changed from the base of the PR and between 5caf2c1 and 4679c8e.

📒 Files selected for processing (3)
  • src/NosCore.Packets/NosCore.Packets.csproj
  • src/NosCore.Packets/ServerPackets/Inventory/EInfoNpcMonsterPacket.cs
  • test/NosCore.Packets.Tests/SerializerTest.cs

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.


Walkthrough

The packet metadata now escapes spaces in NPC monster names during serialization. Tests cover the Fire Cannoneer case. The package version changes from 21.1.1 to 21.1.2.

Changes

Monster name serialization

Layer / File(s) Summary
Serialization contract and validation
src/NosCore.Packets/ServerPackets/Inventory/EInfoNpcMonsterPacket.cs, test/NosCore.Packets.Tests/SerializerTest.cs, src/NosCore.Packets/NosCore.Packets.csproj
EInfoNpcMonsterPacket.Name now escapes spaces as carets. The serializer registers the packet type. A test verifies Fire Cannoneer serializes as Fire^Cannoneer. The package version changes to 21.1.2.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to 4679c

This change corrects how spaces are encoded in one existing monster-name packet field and is covered by a focused serialization test; no actionable merge-blocking risk remains after normal checks and review.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 2 files. (1 skipped: 1… Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: escaping spaces in monster names during packet serialization.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage

Explanation

Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 2 files. (1 skipped: 1 unsupported.)

  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/escape-last-string-field

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@erwan-joly
erwan-joly merged commit c747450 into master Aug 30, 2026
2 of 4 checks passed
@erwan-joly
erwan-joly deleted the feat/escape-last-string-field branch August 30, 2026 22:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant